Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

[RelatedArtists] Use layoutEvent instead of Dimensions #400

Merged
merged 2 commits into from
Nov 30, 2016

Conversation

sarahscott
Copy link
Contributor

Fixes artsy/eigen#2024

I thought this was addressed but I realized it's one of those layout bugs that can only really be seen on-device. I think this is a pattern we should adopt everywhere because it really doesn't seem like Dimensions is a reliable source for layout.

this.state = {
columns: 0,
imageSize: {
width: 1,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opaque_image_view needs some sort of aspect ratio or dimensions to render, so I start with 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to hear suggestions though because this does feel a bit inelegant. I'm guessing this could be fixed by doing some work in the image view or by not rendering images at all until after layout?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alloy ^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can stay as it is for now because the image view will throw an error if you try otherwise.

Copy link
Contributor

@alloy alloy Dec 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the image view wants one or the other so that it can infer if you want to resize or crop. I would say that the simplest solution here is to not render the image view yet until after the onLayout callback is performed and e.g. columnCount > 0.


// TODO: Document what these margins are based on.
let columnCount
let margins
if (isPad) {
if (isPadHorizontal) {
columnCount = 4
margins = 140
margins = 90
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really want to make these calculated the way they are here, so that's why this is still a WIP.

@sarahscott sarahscott changed the title [WIP][RelatedArtists] Use layoutEvent instead of Dimensions [RelatedArtists] Use layoutEvent instead of Dimensions Nov 30, 2016
@sarahscott sarahscott added this to the 3.1.0 milestone Nov 30, 2016
@mennenia
Copy link
Contributor

Nice one!

@mennenia
Copy link
Contributor

It seems something is up with CI (it's timing out) so I'm just gonna go ahead and merge.

@mennenia mennenia merged commit 1b112fb into master Nov 30, 2016
@mennenia mennenia deleted the sarah-related-artists-layout-fix branch November 30, 2016 19:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants